Skip to content

Do not import PETSc at the top-level - #5

Merged
connorjward merged 6 commits into
mainfrom
connorjward/fix-import
Jul 3, 2025
Merged

Do not import PETSc at the top-level#5
connorjward merged 6 commits into
mainfrom
connorjward/fix-import

Conversation

@connorjward

Copy link
Copy Markdown
Collaborator

This avoids us initialising PETSc before petsctools.init is called.

Note that this does result in a small API change. We now have OptionsManager.get_commandline_options() instead of OptionsManager.commandline_options. I don't know a better way to do this.

This avoids us initialising PETSc before petsctools.init is called.
@JHopeCollins

Copy link
Copy Markdown
Member
  1. Could you put an if-guard and a warning around the import to avoid the API change?

  2. Would something like the following work to avoid the API change:

class OptionsManager
    @classmethod
    def _get_commandline_options(cls):
        from petsc4py import PETSc
        return frozenset(PETSc.Options().getAll())

    @property  # or @cachedproperty
    def commandline_options(self):
        return self._get_commandline_options()

@connorjward

Copy link
Copy Markdown
Collaborator Author
  1. Could you put an if-guard and a warning around the import to avoid the API change?
  2. Would something like the following work to avoid the API change:
class OptionsManager
    @classmethod
    def _get_commandline_options(cls):
        from petsc4py import PETSc
        return frozenset(PETSc.Options().getAll())

    @property  # or @cachedproperty
    def commandline_options(self):
        return self._get_commandline_options()

@property does not seem to work with class methods unfortunately. I tried something like this.

Comment thread petsctools/options.py Outdated
Comment thread petsctools/options.py Outdated

@JHopeCollins JHopeCollins left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to DTRT again now for command line arguments. Happy for this to be merged once the linting is passing.

@connorjward
connorjward merged commit bea1a73 into main Jul 3, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants